-
Notifications
You must be signed in to change notification settings - Fork 53
Bugfix/Feature: pascalCase; Bugfix: recursive nesting #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ok, @ardeois should be good for the review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @bohdanbirdie !
I have a few comments before we can merge this.
Especially on the default behaviour that now becomes upper-case
instead of pascal-case
The rest is mostly conceptual
Also note that I'm working on a PR to update all dependencies, so don't worry about updating package versions
README.md
Outdated
}; | ||
return { | ||
get id() { return overrides && 'id' in overrides ? overrides.id! : '0550ff93-dd31-49b4-8c38-ff1cb68bdc38'}, | ||
get url() { return overrides && 'url' in overrides ? overrides.url! : 'aliquid'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you use a getter function?
It seems easier to do:
return {
id: overrides?.hasOwnProperty('id') ? overrides?.id : '0550ff93-dd31-49b4-8c38-ff1cb68bdc38',
url: overrides?.hasOwnProperty('url') ? overrides?.url : 'aliquid',
}
What do you think ?
package.json
Outdated
@@ -43,7 +44,7 @@ | |||
"eslint-plugin-import": "^2.17.2", | |||
"eslint-plugin-jest": "^22.20.0", | |||
"eslint-plugin-prettier": "^3.0.1", | |||
"graphql": "^14.5.8", | |||
"graphql": "^14.6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I'm working on a PR to update all dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to to rever it?
I think I had issues with different versions of graphql so had to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged the update dependencies PR, so if you update your branch you should be good
src/index.ts
Outdated
case 'NonNullType': | ||
return generateMockValue(typeName, fieldName, types, currentType.type); | ||
return generateMockValue(typeName, fieldName, types, currentType.type, enumValues); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in getNamedType
the param enumValues
is before currenType
but in generateMockValue
it's after
Can you place the enumValues
param at the same place for both functions (I don't really mind of it's before or after though)
src/index.ts
Outdated
|
||
return ` ${fieldName}: ${value},`; | ||
return ` get ${fieldName}() { return overrides && '${fieldName}' in overrides ? overrides.${fieldName}! : ${value}},`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like suggested, maybe just do:
return ` ${fieldName}: overrides?.hasOwnProperty('${fieldName}') ? overrides?.${fieldName} : ${value},`;
src/index.ts
Outdated
enumValues, | ||
); | ||
|
||
return ` get ${field.name.value}() { return overrides && '${field.name.value}' in overrides ? overrides.${field.name.value}! : ${value}},`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@bohdanbirdie You should be good to update your branch with master. |
@ardeois ok, updated everything, synced, added test and changed to pascal case as thee default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
This PR is going to fix the issue with pascalCase error, initially committed by @keesvanlierop
Also, I extended that functionality to support upper case enums as well.
The other thing that was failing for me was a recursive nesting.
Check this schema
so, when I create a mock of a task -> user mock will be created, but then user mock will create a task mock and this will run recursively until the stack can't hold it anymore.
Overrides won't help because they come after the mock is created so I switched to the getters, so they won't create a nested mock if there is an override.